Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update helm version #1534

Open
wants to merge 2 commits into
base: kubectl-v20/main
Choose a base branch
from
Open

Conversation

xazhao
Copy link

@xazhao xazhao commented Jan 15, 2025

Note: This helm version is not guaranteed to work with the kubectl version in this branch.

However this layer version is outdated and not supposed to be used anymore. Helm version needs to be updated because it's impacted by the CVE https://security.snyk.io/vuln/SNYK-CHAINGUARDLATEST-HELM-7219926

It's bundled as dependency of CDK. If it's not fixed here, we need to ship a breaking change in main cdk repo which has more impact.

ARG KUBECTL_VERSION=1.20.0
ARG HELM_VERSION=3.8.1
ARG HELM_VERSION=3.17.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're using this layer in aws-cdk-lib if I'm not wrong, any change in here can lead to breaking change for the customers as well as it being used here in CDK.
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/lambda-layer-kubectl/lib/kubectl-layer.ts#L1. Though, EKS natively doesn't support this kubectl version, not sure if we still want to keep it to older version in main lib https://docs.aws.amazon.com/eks/latest/userguide/kubernetes-versions.html.

Copy link
Author

@xazhao xazhao Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this layer is used in aws-cdk-lib and is outdated.
There are 3 ways to resolve it:

  1. make kubectlLayer required from optional
  2. update kubectlLayer default version
  3. update helm version

All of them are breaking changes. 3) is the solution has least impact. Since this layer is not supported natively by EKS, the change impact is minimal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar with change as this PR #623

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue (1) has the least impact. Yes, it will break builds. But at least it will break them transparently. Both (2) and (3) might or might not break a customer depending on the exact circumstances. However if it does, the failure will be much less transparent and a few steps removed. (1) has a guarantee to have zero impact on deployed resources. With (2) & (3) we have the potential to cause an outage to a customer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants